Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make it directly usable in Docker #1336

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
Open

Conversation

katomaso
Copy link

@katomaso katomaso commented Apr 8, 2024

Hi, I am trying to use your app semi-production in Docker and this is necessary changes to be able to run it painlessly on stock Ubuntu server with generic database.

Are any of those changes interesting for you that you would consider porting them to your code?

@cellio cellio requested a review from a team April 9, 2024 23:10
@cellio
Copy link
Member

cellio commented Apr 9, 2024

@Taeir I'm not directly assigning you as a reviewer (though qpixel-reviewers should cover you), but pinging you in case you have both specific knowledge here and time to review. If not, don't worry about it -- this is why we have a reviewers group. :-)

@cellio cellio added the area: deployment Issues in setting up qpixel, containers, etc. label Apr 18, 2024
@cellio cellio requested a review from sau226 May 5, 2024 02:04
@tsengia
Copy link

tsengia commented Jun 23, 2024

I am very interested in this PR. Being able to deploy via Docker would be great! Let me know if I can be of help somehow.

@cellio
Copy link
Member

cellio commented Jun 28, 2024

@ArtOfCode- would you be able to help shepherd this along? I don't know if you're the right reviewer but you have a better sense of this than I do. Thanks.

@tsengia thanks for your patience. Note that our Docker setup wasn't intended for production systems and you might run into some issues. Most notably, Redis data isn't stored outside of Docker, and Redis is where we store some state info (like when you last viewed a category) and also some user preferences. Qpixel also can't send email from inside Docker. For more caveats, see https://collab.codidact.org/posts/289900/289902#answer-289902 . You said "semi-production", so maybe none of these are issues for you, but I just wanted to make sure you know.

I don't know what it would take to change any of these restrictions; if you have ideas on how to make that better, we'd welcome PRs. But this is where it stands now to the best of my limited knowledge.

@cellio
Copy link
Member

cellio commented Jul 8, 2024

@katomaso I'm sorry for the delay. This PR includes both Docker changes and other changes, but it's not clear (to me at least) how the other changes are related. Can you say more about how the Ruby and db/seeds changes are related to the Docker issue? Or are they unrelated, and we could merge the Docker changes independently of the others? Thanks.

RUN gem install bundler -v 2.4.22 && \
bundle install

COPY . /code
RUN bundle exec rails assets:precompile && \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB!

If anyone decides to port this change, please note that we do not have (yet) a separate Dockerfile for development and precompiled assets are not excluded from git tracking via .gitignore, meaning that without changing the above, doing so will heavily pollute the working directory.

rails db:seed

# defaults to port 3000
echo "Starting server 0.0.0.0:3000"
Copy link
Member

@Oaphi Oaphi Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB!

If anyone decides to port this change, do note that the port is configurable, so without making it a variable, this will likely report an incorrect port (I have qpixel running on 3014 myself, for example).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Quick question - can you clarify a little as to what was the reasoning behind removing this migration? I presume there was a compelling reason for doing so, but lack context.

ENV NODEJS_HOME=/opt/node-v12.18.3-linux-x64/bin
ENV PATH=$NODEJS_HOME:$PATH
libgdbm-dev \
nodejs \
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NB!

If anyone decides to port this change, please, review that the Node.js version pulled is compatible or, better, yet, fix it to a specific version range (as we should do with all other dependencies, tbh).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: deployment Issues in setting up qpixel, containers, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants